Introduce BytesTransportResponse to enable lower heap overhead network responses #127112
Conversation
This introduces a way of sending transport responses without materializing an actual response object. This gets a little more complicated for responses that can both go to the local direct response channel as well as to the wire, but for the per-node query response that isn't sent locally we can use it to save creating the response object. The version here only saves a small amount of heap yet (though it makes the GC's life a little easier) but this can be taken further step by step easily by also e.g. streaming aggregation results into the output stream instead of ever materializing them. Another candidate that could win big from this change is the fetch phase where we could effectively halve the peak heap use by serializing directly.
original-brownbear
added
>non-issue
:Distributed Coordination/Network
elasticsearchmachine
added
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| var result = queryPhaseResultConsumer.results.get(i); | ||
| if (result == null) { | ||
| out.writeBoolean(false); | ||
| out.writeException(failures.remove(i)); |
There was a problem hiding this comment.
This kind of trick is neatly enabled if we serialize the response inline, makes the lifecycle of the failures map entirely obvious. The same trick could be played on the individual responses but it's a little more complicated and I didn't want to muddy the waters with a tricky change to the ref-counting by doing it in here.
| out = dependencies.transportService.newNetworkBytesStream(); | ||
| out.setTransportVersion(channel.getVersion()); | ||
| try { | ||
| out.writeVInt(resultCount); |
There was a problem hiding this comment.
Unfortunately we need to duplicate the writeTo() on the message for the time being because of CCS proxy request BwC concerns urgh :) but I think it's not too bad and we can dry this up in a follow-up. This change is about networking, not search really so I didn't want to mess with that here.
There was a problem hiding this comment.
Transport changes LGTM. Not so sure about the response serialization stuff - it looks like the right sort of shape, but either this bit of the PR needs a search team review or else a separate PR to dry this code up first so that we can make this PR more obviously correct.
| ActionListener.respondAndRelease(channelListener, new BytesTransportResponse(new ReleasableBytesReference(out.bytes(), out))); | ||
| } | ||
|
|
||
| private void maybeFreeContext(SearchPhaseResult result, BitSet relevantShardIndices) { |
There was a problem hiding this comment.
This wants to live in QueryPhaseResultConsumer in the near term anyway :) We can release contexts earlier than just at the end of the per-node action if we merge the top hits more frequently.
|
Made it as dry/similar as I could :) thanks David! |
|
Sorry mised the separte PR part :) I'll find someone from search ;) Thanks again David! |
|
Thanks so much you two! good stuff incoming based on this :) |
…ode (#135873) #127112 introduced `BytesTransportResponse` to be used in search batched execution, so that `NodeQueryResponse` could be written as `BytesTransportResponse` as opposed to materializing the response object on heap. When a proxy node acts as a proxy to query its local data, and the coordinating node is on a different version than the proxy node, the response will fail to deserialize in the coord node because it was written with the version of the proxy node as opposed to that of the coord (target) node. This is because `DirectResponseChannel` skips the step of reading and writing back such response, which would lead to it being converted to the right format. This commit attempts to fix this problem by tracking the version used to write the binary response, and conditionally converting it in the `ProxyRequestHandler` when the version don't align.
5 participants
In a sense the logical continuation of #125163 removing heap retention of pieces of a response after that PR dealt with retaining the response object.
This introduces a way of sending transport responses without materializing an actual response object. This gets a little more complicated for responses that can both go to the local direct response channel as well as to the wire, but for the per-node query response that isn't sent locally we can use it to save creating the response object.
The version here only saves a small amount of heap yet (though it makes the GC's life a little easier) but this can be taken further step by step easily by also e.g. streaming aggregation results into the output stream instead of ever materializing them.
Another candidate for a quick follow-up that would win big from this change's infrastructure is the fetch phase where we could effectively halve the peak heap use by serializing directly and building a compound response that contains the unpooled buffers that we have for fetched sources anyways.